kvstreamer: fix a bug with incorrect processing of empty scan responses #75888
Conversation
|
This change is |
yuzefovich
force-pushed
the
streamer-debug
branch
13 times, most recently
from
79fe38f to
ed94654
Compare
yuzefovich
changed the title
yuzefovich
force-pushed
the
streamer-debug
branch
from
ed94654 to
a5c9927
Compare
yuzefovich
changed the title
yuzefovich
force-pushed
the
streamer-debug
branch
3 times, most recently
from
1e5f570 to
56a56ac
Compare
There was a problem hiding this comment.
Good fix. I have some initial comments, but still need to review most of the second commit.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rytaft and @yuzefovich)
pkg/sql/row/kv_batch_streamer.go, line 41 at r1 (raw file):
"Enabling this will increase the speed of lookup/index joins "+ "while adhering to memory limits.", true,
Did you mean to turn this on by default in this bug fix? If so, this should be called out in the commit message at least.
Also, when it is enabled by default, it would be good to keep some test coverage where it is disabled to make sure the other code path is still operational -- unless you think that the operations that don't support the streamer yet provide sufficient coverage.
pkg/kv/kvclient/kvstreamer/streamer_test.go, line 461 at r1 (raw file):
var union roachpb.RequestUnion_Scan makeKey := func(pk int) []byte { return []byte{190, 137, byte(136 + pk)}
Out of curiosity...what are these magic numbers?
yuzefovich
force-pushed
the
streamer-debug
branch
from
56a56ac to
81ac414
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/row/kv_batch_streamer.go, line 41 at r1 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
Did you mean to turn this on by default in this bug fix? If so, this should be called out in the commit message at least.
Also, when it is enabled by default, it would be good to keep some test coverage where it is disabled to make sure the other code path is still operational -- unless you think that the operations that don't support the streamer yet provide sufficient coverage.
Yes, I did mean to turn it on by default since the first commit fixes the only known issue. Updated the commit message to include this note.
I'm not really concerned about the test coverage of the "streamer off" case because that code path is essentially everything that goes through the SQL layer and touches KV whereas the streamer code path is specific to only index joins in some cases.
pkg/kv/kvclient/kvstreamer/streamer_test.go, line 461 at r1 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
Out of curiosity...what are these magic numbers?
These make up a /t/primary/pk key, added a comment.
There was a problem hiding this comment.
Reviewable status:
yuzefovich
force-pushed
the
streamer-debug
branch
from
81ac414 to
af9a0f7
Compare
Previously, the `Streamer` could get stuck indefinitely when a response for a Scan request came back empty - namely the response neither had no bytes inside nor ResumeSpan set (this is the case when there is no data in the key span to scan). This would lead to `GetResults` call being stuck thinking there are more responses to come back, and none would show up. This is now fixed by creating a Result with an empty Scan response inside of it. This approach makes it easier to support Scans that span multiple ranges and the last range has no data in it - we want to be able to set Complete field on such an empty Result. Since this was the only known bug with the `Streamer` implementation, the streamer is now used by default again. Release note: None
yuzefovich
force-pushed
the
streamer-debug
branch
from
af9a0f7 to
c0373e6
Compare
This commit replaces the last buffered channel we had in the `Streamer` code in favor of a condition variable which simplifies the control flow a bit. Additionally, this commit refactors the code so that empty Get responses are not returned which allows us to clean up `TxnKVStreamer` a bit. Care has to be taken so that we still signal the client's goroutine waiting for results when an empty Get response comes back (similar to the bug fixed by the previous commit). Also, the tracking of the "number of outstanding requests" in `TxnKVStreamer` has been removed since it provided little value (and probably was broken anyway). Release note: None
yuzefovich
force-pushed
the
streamer-debug
branch
from
c0373e6 to
4cc956f
Compare
|
TFTR! bors r+ |
|
Build succeeded: |
kvstreamer: fix a bug with incorrect processing of empty scan responses
Previously, the
Streamercould get stuck indefinitely when a responsefor a Scan request came back empty - namely the response neither had no
bytes inside nor ResumeSpan set (this is the case when there is no data
in the key span to scan). This would lead to
GetResultscall beingstuck thinking there are more responses to come back, and none would
show up.
This is now fixed by creating a Result with an empty Scan response
inside of it. This approach makes it easier to support Scans that span
multiple ranges and the last range has no data in it - we want to be
able to set Complete field on such an empty Result.
Since this was the only known bug with the
Streamerimplementation,the streamer is now used by default again.
Fixes: #75708.
Release note: None
kvstreamer: minor cleanup
This commit replaces the last buffered channel we had in the
Streamercode in favor of a condition variable which simplifies the control flow
a bit.
Additionally, this commit refactors the code so that empty Get responses
are not returned which allows us to clean up
TxnKVStreamera bit. Carehas to be taken so that we still signal the client's goroutine waiting
for results when an empty Get response comes back (similar to the bug
fixed by the previous commit).
Also, the tracking of the "number of outstanding requests" in
TxnKVStreamerhas been removed since it provided little value (andprobably was broken anyway).
Release note: None